feat: dont call spawn cb more than once on error #243

dryajov · 2018-05-09T07:10:39Z

No description provided.

vasco-santos · 2018-05-14T17:38:55Z

As a new release occurred today, could you fix the introduced merge conflict @dryajov ?

dryajov · 2018-05-14T17:55:24Z

@vasco-santos done - conflicts resolved.

daviddias · 2018-05-14T19:22:26Z

test/spawn-options.spec.js

@@ -160,7 +161,6 @@ describe('Spawn options', function () {
      })
    })

-    // TODO ?? What is this test trying to prove?


Why delete?

It tries to spawn a node an then manually attach an ipfs-api to it - we do that in some places in ipfs-api tests and others...

daviddias · 2018-05-14T19:22:33Z

src/ipfsd-in-proc.js

@@ -72,6 +72,8 @@ class Node extends EventEmitter {
      libp2p: this.opts.libp2p
    })

+    // TODO: should this be wrapped in a process.nextTick(), for context:
+    // https://nodejs.org/en/docs/guides/event-loop-timers-and-nexttick/#why-use-process-nexttick


Why not do it now?

I'm asking for feedback there mostly - did you take a look at the link?

daviddias · 2018-05-14T19:23:17Z

src/factory-in-proc.js

+    ], (err) => {
+      node.removeListener('error', errHandler)
+      if (!called) { callback(err, node) }
+    })


Why not use the dependency once you also added in this PR?

daviddias · 2018-05-14T19:24:06Z

package.json

@@ -92,7 +92,6 @@
    "lodash.defaults": "^4.2.0",


Update ipfs-repo to 0.20.0 and update all deps below 1.0.0 to be ~ rather then ^

daviddias · 2018-05-14T19:35:17Z

Can I get a memory refresher on why the following error is currently happening?

Debug: internal, implementation, error
    AssertionError: expected [Error: invalid block] to not exist
    at f.spawn (/Users/imp/code/js-ipfsd-ctl/test/add-retrieve.spec.js:29:28)
    at Node.errHandler (/Users/imp/code/js-ipfsd-ctl/src/factory-in-proc.js:136:7)
    at Object.onceWrapper (events.js:315:30)
    at emitOne (events.js:116:13)
    at Node.emit (events.js:211:7)
    at IPFS.Node.exec.once.err (/Users/imp/code/js-ipfsd-ctl/src/ipfsd-in-proc.js:77:41)

daviddias · 2018-05-14T19:56:54Z

The added test is failing, both locally and on CI

      don't callback twice on error
Debug: internal, implementation, error
    TypeError: ipfsd.once is not a function
    at once.strict (/Users/imp/code/js-ipfsd-ctl/test/spawn-options.spec.js:452:17)
    at f (/Users/imp/code/js-ipfsd-ctl/node_modules/once/once.js:36:25)
    at series (/Users/imp/code/js-ipfsd-ctl/src/factory-daemon.js:144:7)
    at /Users/imp/code/js-ipfsd-ctl/node_modules/async/internal/parallel.js:39:9
    at /Users/imp/code/js-ipfsd-ctl/node_modules/async/internal/once.js:12:16
    at replenish (/Users/imp/code/js-ipfsd-ctl/node_modules/async/internal/eachOfLimit.js:59:25)
    at iterateeCallback (/Users/imp/code/js-ipfsd-ctl/node_modules/async/internal/eachOfLimit.js:49:17)
    at /Users/imp/code/js-ipfsd-ctl/node_modules/async/internal/onlyOnce.js:12:16
    at /Users/imp/code/js-ipfsd-ctl/node_modules/async/internal/parallel.js:36:13
    at f (/Users/imp/code/js-ipfsd-ctl/node_modules/once/once.js:25:25)
    at Socket.data (/Users/imp/code/js-ipfsd-ctl/src/ipfsd-daemon.js:281:11)
    at emitOne (events.js:121:20)
    at Socket.emit (events.js:211:7)
    at addChunk (_stream_readable.js:263:12)
    at readableAddChunk (_stream_readable.js:250:11)
    at Socket.Readable.push (_stream_readable.js:208:10)
    at Pipe.onread (net.js:607:20)
        1) spawn with error

daviddias · 2018-05-14T20:09:06Z

Just missing #243 (comment). @dryajov go ahead and pull the latest changes and apply the CR + fix the test. Then we will have CI green :)

… ipfs-repo" This reverts commit 42597c1.

dryajov · 2018-05-14T21:54:18Z

I've reverted back to using ipfs-repo because of (from IRC):

15:19 <dryajov> daviddias: seems like we need ipfs-repo after all… 
15:19 <dryajov> here is why: we need to check if the repo exists, close/cleanup repo after we finished
15:19 <dryajov> and we need to be able to do it across node and browser...
15:20 <dryajov> thats the catch...
15:20 <dryajov> we don’t need to create it (just let ipfs take care of that), but we need it for cleanup/checking if it exists
15:21 <dryajov> we can do all that manually, but, I’m sure we’ll run into nasty corner cases...

daviddias

The ipfs-repo is definitely not required as a main dependency, nor it seems that useful for tests, in fact, it might mislead the developer to think the repo was closed properly.

daviddias · 2018-05-15T09:47:33Z

package.json

@@ -86,22 +86,22 @@
    "hapi": "^16.6.2",
    "hat": "0.0.3",
    "ipfs-api": "^21.0.0",
-    "ipfs-repo": "^0.19.0",
+    "ipfs-repo": "~0.20.0",


@dryajov even with #243 (comment), the teardown is just used for testing. It should not be a regular dependency nor be a util from src.

@diasdavid it's not only used in tests, we use it the cleanup method as well -

js-ipfsd-ctl/src/ipfsd-in-proc.js

Line 181 in b7ef098

this.repo.teardown(callback)

AFAIK, when calling ipfs.stop(), the repo doesn't get cleaned up, hence we need something to be able to purge it from the browsers storage (indexDB, websql, etc...), not doing that would give a false sense of the repo being cleaned up/deleted when its not, and doing it by hand would possibly lead us to deal with browser inconsistencies and other corner cases that are most likely taken care already by ipfs-repo.

I might be missing something in my assessment, would be happy to hear otherwise 👍

OK, slight correction - seems like the only place we would need ipfs-repo would be in

js-ipfsd-ctl/src/factory-in-proc.js

Line 144 in 21b9850

(cb) => node.repo._isInitialized(err => {

I'll take a closer look how to better handle that piece, once thats out it should be easy to remove ipfs-repo.

daviddias · 2018-05-15T09:50:03Z

src/utils/repo/create-browser.js

+  }
+
+  return repo
+}


Also based on #243 (comment), this is an antipattern. What this is creating is that IPFS might fail to close the repo and we are forcely closing it and deleting.

The only things you should need for tests are the idb.deleteDatabase and rimraf calls

vasco-santos · 2018-05-17T09:22:08Z

@dryajov @diasdavid can we go forward with this to have the CI green again?

daviddias · 2018-05-17T09:45:47Z

Just missing Circle CI :)

dryajov · 2018-05-17T15:23:39Z

Hnm. Circle looks weird, I'll take a look.

dryajov · 2018-05-17T16:35:14Z

@diasdavid @vasco-santos can I get write perms so I can trigger rebuilds on circle and other CIs?

vasco-santos · 2018-05-17T16:39:00Z

Fine by me! But I am not able to give you permissions, at least AFAIK

dryajov · 2018-05-17T18:53:30Z

ok, circle is green.

ghost assigned dryajov May 9, 2018

ghost added the status/in-progress In progress label May 9, 2018

dryajov mentioned this pull request May 9, 2018

Spawn callback can be called multiple times #239

Closed

dryajov force-pushed the fix/spawn-err-callback branch from 6a58649 to 4743451 Compare May 9, 2018 16:11

vasco-santos previously approved these changes May 10, 2018

View reviewed changes

fsdiogo previously approved these changes May 10, 2018

View reviewed changes

dryajov added 3 commits May 14, 2018 11:45

feat: dont call spawn cb more than once on error

46ed8fa

fix: add flag to signal callback was already called

1f0b736

docs: adding TODO

e0fc124

dryajov dismissed stale reviews from fsdiogo and vasco-santos via e0fc124 May 14, 2018 17:46

dryajov force-pushed the fix/spawn-err-callback branch from 4743451 to e0fc124 Compare May 14, 2018 17:46

daviddias requested changes May 14, 2018

View reviewed changes

chore: fix deps semvers

fd13f5a

ghost assigned daviddias May 14, 2018

test: skip inProc for add-retrieve until js-ipfs 0-29

825f74a

feat: let js-ipfs create a repo for itself, no need to import ipfs-repo

42597c1

dryajov added 2 commits May 14, 2018 15:39

Revert "feat: let js-ipfs create a repo for itself, no need to import…

b7ef098

… ipfs-repo" This reverts commit 42597c1.

fix: use once to call cb once

21b9850

test: skip in proc tests untill js-ipfs 0.29 release is out

0ed2e96

daviddias requested changes May 15, 2018

View reviewed changes

daviddias and others added 3 commits May 15, 2018 10:55

chore: use 8.11.1 for circle

ffa36a2

feat: detect inited repo, remove ipfs-repo

cf6f865

fix: remove TODOs

408b96f

daviddias approved these changes May 17, 2018

View reviewed changes

daviddias merged commit ca44996 into master May 17, 2018

daviddias deleted the fix/spawn-err-callback branch May 17, 2018 18:58

ghost removed the status/in-progress In progress label May 17, 2018

vasco-santos mentioned this pull request May 30, 2018

Make CI green again #242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dont call spawn cb more than once on error #243

feat: dont call spawn cb more than once on error #243

dryajov commented May 9, 2018

vasco-santos commented May 14, 2018

dryajov commented May 14, 2018

daviddias May 14, 2018

dryajov May 14, 2018

daviddias May 14, 2018

dryajov May 14, 2018

daviddias May 14, 2018

daviddias May 14, 2018

daviddias commented May 14, 2018

daviddias commented May 14, 2018

daviddias commented May 14, 2018

dryajov commented May 14, 2018

daviddias left a comment

daviddias May 15, 2018 •

edited

Loading

dryajov May 15, 2018

dryajov May 15, 2018

daviddias May 15, 2018

vasco-santos commented May 17, 2018

daviddias commented May 17, 2018

dryajov commented May 17, 2018

dryajov commented May 17, 2018 •

edited

Loading

vasco-santos commented May 17, 2018

dryajov commented May 17, 2018

feat: dont call spawn cb more than once on error #243

feat: dont call spawn cb more than once on error #243

Conversation

dryajov commented May 9, 2018

vasco-santos commented May 14, 2018

dryajov commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented May 14, 2018

daviddias commented May 14, 2018

daviddias commented May 14, 2018

dryajov commented May 14, 2018

daviddias left a comment

Choose a reason for hiding this comment

daviddias May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented May 17, 2018

daviddias commented May 17, 2018

dryajov commented May 17, 2018

dryajov commented May 17, 2018 • edited Loading

vasco-santos commented May 17, 2018

dryajov commented May 17, 2018

daviddias May 15, 2018 •

edited

Loading

dryajov commented May 17, 2018 •

edited

Loading